Skip to content

Extend Configurables to include a startup file#560

Merged
robertopreste merged 9 commits into
masterfrom
555-extend-configurables
Aug 11, 2021
Merged

Extend Configurables to include a startup file#560
robertopreste merged 9 commits into
masterfrom
555-extend-configurables

Conversation

@robertopreste

Copy link
Copy Markdown
Contributor

Closes #555

This PR adds a new Configurable, ParaviewData, that can exploit the SRDATA env variable exposed by Paraview-S-R to open a specific file when ParaView starts up.

The Admin user can set a policy where this functionality is enabled, in which case the User will be shown a "Startup file" text input in the main view, where the path to a startup file can be provided. Currently, the only functional path is /ParaView/can.ex2, which is an example file included in the ParaView Docker image.

@codecov-commenter

codecov-commenter commented Aug 10, 2021

Copy link
Copy Markdown

Codecov Report

Merging #560 (dec92c8) into master (4b07ecd) will increase coverage by 0.06%.
The diff coverage is 100.00%.

❗ Current head dec92c8 differs from pull request most recent head 95611f1. Consider uploading reports for the commit 95611f1 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #560      +/-   ##
==========================================
+ Coverage   95.10%   95.16%   +0.06%     
==========================================
  Files          92       92              
  Lines        4168     4222      +54     
  Branches      269      271       +2     
==========================================
+ Hits         3964     4018      +54     
  Misses        144      144              
  Partials       60       60              
Impacted Files Coverage Δ
remoteappmanager/db/tests/test_csv_db.py 100.00% <ø> (ø)
remoteappmanager/db/tests/test_interfaces.py 100.00% <ø> (ø)
remoteappmanager/tests/mocking/dummy.py 91.66% <ø> (ø)
...eappmanager/tests/mocking/virtual/docker_client.py 93.04% <ø> (ø)
...teappmanager/webapi/admin/tests/test_accounting.py 100.00% <ø> (ø)
remoteappmanager/webapi/tests/test_application.py 100.00% <ø> (ø)
remoteappmanager/cli/tests/test_remoteappdb.py 100.00% <100.00%> (ø)
remoteappmanager/db/csv_db.py 98.78% <100.00%> (+0.01%) ⬆️
remoteappmanager/db/interfaces.py 100.00% <100.00%> (ø)
remoteappmanager/db/orm.py 94.23% <100.00%> (+0.02%) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b07ecd...95611f1. Read the comment docs.

@flongford flongford left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @robertopreste! This generally looks good, I just have some comments about variable names since ideally we don't want to refer to any specific application (i.e. ParaView) in the framework itself.

Obviously, the ParaView application is currently the only image that can make use of this feature, but in theory any other app image could also use the SRDATA environmental variable to refer to a startup data file.

Also, I now remember that we previously discussed how an admin user may like to configure the environmental variable name itself (i.e. use $MY_APP_DATA instead of $SRDATA). For the time being I think we can just use a fixed variable name for all applications, though it would be nice to keep in mind that we may want to extend this in future.

Comment thread frontend/user/configurables/ParaviewData.vue Outdated
Comment thread frontend/user/configurables/configurables.js Outdated
Comment thread remoteappmanager/cli/remoteappdb/__main__.py Outdated
Comment thread remoteappmanager/cli/remoteappdb/__main__.py Outdated
Comment thread remoteappmanager/docker/configurables.py
Comment thread remoteappmanager/docker/configurables.py Outdated

@flongford flongford left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of nits, otherwise LGTM!

Comment thread doc/source/developer/docker.rst Outdated
Comment thread remoteappmanager/docker/configurables.py Outdated
@robertopreste

Copy link
Copy Markdown
Contributor Author

Couple of nits, otherwise LGTM!

Right, thanks for catching these points, I totally overlooked them!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Extend Simphony-Remote Configurables to specify an “input” file for application container

3 participants